Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use no-param-reassign's ignorePropertyModificationsFor option #1325

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

christianbundy
Copy link
Contributor

@christianbundy christianbundy commented Mar 3, 2017

This adds exceptions to no-param-reassign to resolve #1217. There's probably some discussion to be done around which variables should be ignored, but currently I've added req, request, and $scope from this comment.

Please let me know if you have any questions, or if there are any changes you'd like for me to make. Thanks!

Closes #1217

// rule: http://eslint.org/docs/rules/no-param-reassign.html
'no-param-reassign': ['error', { props: true }],
'no-param-reassign': ['error', { props: true, ignorePropertyModificationsFor: ['req', 'request', '$scope'] }],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's include acc for reduce accumulators, and res and response for express responses, and e for e.returnValue =.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also let's split the options object (and the array) onto multiple lines, so that each exclusion can get a comment describing why it's in the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I resolved this in 5d3c171 by adding those 4 exceptions and adding comments for each.

@@ -164,9 +164,9 @@ module.exports = {
'no-octal-escape': 'error',

// disallow reassignment of function parameters
// disallow parameter object manipulation
// disallow parameter object manipulation except req, request, and $scope
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should rename generic, ie, "except for specific exclusions"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks! Resolved in 940d598.

@christianbundy
Copy link
Contributor Author

@ljharb Are there any other steps I should take to get this merge-ready?

@ljharb
Copy link
Collaborator

ljharb commented Mar 14, 2017

@christianbundy Awesome! Can we add ctx for koa routing as well? Other than that, we're good to go :-)

@christianbundy
Copy link
Contributor Author

Done! Let me know if you'd like to squash my commits before the merge, thanks for working with me on this PR.

@ljharb
Copy link
Collaborator

ljharb commented Mar 15, 2017

@christianbundy yes, please do rebase it down to one commit :-) please also prefix the commit message with [eslint config] [base] . Thanks!

@christianbundy
Copy link
Contributor Author

Weird, this page started throwing HTTP 500 errors after I force-pushed, and it looks like it auto-closed? Anyway, I re-opened it with the new commit message, please let me know if there's anything else.

@ljharb ljharb merged commit 3475264 into airbnb:master Mar 16, 2017
@coldfish
Copy link

After this changes, I'm getting this error:

Failed to compile.
Error in ./src/actions/actions.js
Module build failed: Error: \node_modules\eslint-config-airbnb-base\rules\best-practices.js:
Configuration for rule "no-param-reassign" is invalid:
Value "data["0"].ignorePropertyModificationsFor" has additional properties.
Referenced from: \node_modules\eslint-config-airbnb-base\index.js
Referenced from: airbnb
Referenced from: .eslintrc.js
at Array.forEach (native)
at Array.reduceRight (native)
at Array.reduceRight (native)
at Array.reduceRight (native)

I have these versions in my package.json:

"eslint": "3.15.0",
"eslint-config-airbnb": "^14.1.0",
"eslint-config-react-app": "^0.5.2",
"eslint-loader": "1.6.0",
"eslint-plugin-flowtype": "2.21.0",
"eslint-plugin-import": "2.2.0",
"eslint-plugin-jsx-a11y": "^4.0.0",
"eslint-plugin-react": "6.9.0",

If I change back to the oldest version, everything works properly. Do you have any suggestions about it?

@christianbundy
Copy link
Contributor Author

christianbundy commented Mar 27, 2017

Hey @coldfish, do you get any errors when you run npm install? You should be getting a warning for that ESLint package, since ESLint v3.18.0 is listed as a peer dependency. This functionality was released in ESLint v3.17.0, would it be possible to upgrade to the most recent version of ESLint?

@coldfish
Copy link

coldfish commented Mar 27, 2017

After npm install, I only got this WARN message:

npm WARN eslint-config-react-app@0.5.2 requires a peer of eslint-plugin-jsx-a11y@^2.2.3 but none was installed.

Based on the installation documentation (https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb), I have installed these versions after getting the output of "npm info "eslint-config-airbnb@latest" peerDependencies":

{ eslint: '^3.15.0',
  'eslint-plugin-jsx-a11y': '^3.0.2 || ^4.0.0',
  'eslint-plugin-import': '^2.2.0',
  'eslint-plugin-react': '^6.9.0' }

So I thought they're the right dependencies to proceed. Should we have update the documentation for this problem?

Thanks for your quick reply.

@christianbundy
Copy link
Contributor Author

It looks like you're right, that eslint-config-airbnb package has a minimum peer dependency of ESLINT v3.16.1, but if you want to use ESLint's newer features you'll need to upgrade to a more recent version of ESLint. I'm not on the Airbnb team, but I assume this is similar to how you wouldn't be able to use ES6-specific styles unless your JS engine was capable.

The peerDependency you mentioned above is ^3.15.0, which is equivalent to 3.x.x. It looks like you may have accidentally locked the specific versions rather than used the caret, can you try this?

npm install --save-dev eslint@^3.15.0 eslint-plugin-jsx-a11y@^4.0.0 eslint-plugin-import@^2.2.0 eslint-plugin-react@^6.9.0

@coldfish
Copy link

Thanks @christianbundy. This answer is pretty enough for me. As you said, most probably, I've made a mistake while installing the packages related to caret.

After updating the version to 3.18.0, the error has gone.

@ljharb
Copy link
Collaborator

ljharb commented Mar 27, 2017

Also note, when updating eslint-config-airbnb or eslint-config-airbnb-base, please always use the install command in the package readme, so the peer dep versions end up correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why assignment to property of function parameter is bad style.
4 participants